Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add application message priority default #578

Merged
merged 8 commits into from
Jul 19, 2023
Merged

Conversation

chrispruitt
Copy link
Contributor

This PR adds the feature discussed in #312.

It allows a user to set a message priority default at the application level. If a sender includes a priority in the request payload to create a message, then the default will be ignored. If the sender omits the priority, then the default priority is applied to the message.

Any feedback is appreciated!

Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, I've added remarks in subcomments.

api/application.go Outdated Show resolved Hide resolved
api/application.go Outdated Show resolved Hide resolved
api/message.go Outdated Show resolved Hide resolved
model/application.go Outdated Show resolved Hide resolved
test/testdb/database.go Outdated Show resolved Hide resolved
ui/src/application/AppStore.ts Outdated Show resolved Hide resolved
ui/src/application/UpdateApplicationDialog.tsx Outdated Show resolved Hide resolved
ui/src/application/UpdateApplicationDialog.tsx Outdated Show resolved Hide resolved
ui/src/application/UpdateApplicationDialog.tsx Outdated Show resolved Hide resolved
api/application_test.go Show resolved Hide resolved
@chrispruitt
Copy link
Contributor Author

I updated the logic to only use the default priority for a truly omitted priority. This will allow a client to send a message with any priority (including 0 this time). I did this by making the ExternalMessage.Priority field a pointer which would then be nil if the priority wasn't included in the request. Thoughts?

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 🎉

Comparison is base (aedc3e2) 86.09% compared to head (7bfba2f) 86.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #578      +/-   ##
==========================================
+ Coverage   86.09%   86.16%   +0.06%     
==========================================
  Files          45       45              
  Lines        1582     1590       +8     
==========================================
+ Hits         1362     1370       +8     
  Misses        138      138              
  Partials       82       82              
Impacted Files Coverage Δ
api/application.go 82.90% <100.00%> (+0.29%) ⬆️
api/message.go 84.46% <100.00%> (+0.46%) ⬆️
plugin/manager.go 92.11% <100.00%> (ø)
plugin/messagehandler.go 100.00% <100.00%> (ø)
test/testdb/database.go 97.01% <100.00%> (+0.13%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chrispruitt
Copy link
Contributor Author

@jmattheis thanks for your help on this one. The number field is much better the way you implemented it. I rebased the branch to be in sync with master now and added a test to bump the coverage back up. Hoping all checks will pass on this next run. Thanks!

Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@jmattheis jmattheis merged commit 72bd8c8 into gotify:master Jul 19, 2023
3 checks passed
@tessus
Copy link
Contributor

tessus commented Jul 20, 2023

I think the gotify client needs a change for this as well. During init the client stores a default priority. There is no way to disregard sending a default priority.

Otherwise, I like the idea of having a priority on application level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants